Hello @dcherian! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-06-24 21:51:22 UTC

Thanks for putting this together.

I think this can work as long as time has an encoding - however, if it doesn't we might end up with different units for time and time_bounds again. I am not sure, actually, just something to check.

Yeah, this still doesn't work as @klindsay28 just pointed out to me.

I still don't understanding how the units attribute/encoding is being treated. I'll get back to this soon.

Thanks @spencerkclark, I'm going by the example in #2921. I've added that now as a test. I also test for the slight non-compliance case where time_bounds has different units already set.

@mathause would this test catch the issue you were encountering?

Great - this does indeed solve my problem (read multiple files with open_mfdataset and write as one). However, it still 'fails' - is not cf-compiant to be more precise - for a new dataset created in xarray, see:

import xarray as xr
import pandas as pd
time = pd.date_range('2000-01-16', periods=1)
time_bounds = pd.date_range('2000-01-01', periods=2, freq='MS')

ds = xr.Dataset(dict(time=time, time_bounds=time_bounds))
ds.time.attrs['bounds'] = 'time_bounds'

xr.conventions.cf_encoder(ds.variables, ds.attrs)

(compare the units). This might be difficult to solve, as xarray currently assumes all variables can be encoded independently.

This might be difficult to solve, as xarray currently assumes all variables can be encoded independently.

I was hoping someone wouldn't bring that case up :). But I agree I think it's something we should discuss.

I'm sort of torn on whether it is worth the complexity. Part of me feels like if a user is concerned/clever enough to explicitly create such linking attributes (e.g. 'bounds' pointing to 'time_bounds') in their Datasets that we could leave it as their responsibility to make sure the 'units' and 'calendar' encodings match for each of the variables as well. For example, while a bit verbose, this would work as desired:

import xarray as xr
import pandas as pd
time = pd.date_range('2000-01-16', periods=1)
time_bounds = pd.date_range('2000-01-01', periods=2, freq='MS')

ds = xr.Dataset(dict(time=time, time_bounds=time_bounds))
ds.time.encoding['bounds'] = 'time_bounds'
ds.time.encoding['units'] = 'days since 2000-01-01'
ds.time.encoding['calendar'] = 'proleptic_gregorian'

ds.time_bounds.encoding['units'] = ds.time.encoding['units']
ds.time_bounds.encoding['calendar'] = ds.time.encoding['calendar']

That said, that case is something that in principle we could address, and maybe it is worth thinking about if we ever consider increasing the functionality related to cell bounds coordinates (e.g. #1475).

Yes, I agree - I don't intend to actually create such a dataset by hand anytime soon ;) but it would be super hard to track this issue down - can we issue a warning instead? See my inline comment.

This case might actually be really easy to fix since we already have xr.conventions._update_bounds_attributes() from Fabien's previous PR (see below).

I think we would just need to call this prior to calling encode_cf_variable() in

def _update_bounds_attributes(variables):
    """Adds time attributes to time bounds variables.

    Variables handling time bounds ("Cell boundaries" in the CF
    conventions) do not necessarily carry the necessary attributes to be
    decoded. This copies the attributes from the time variable to the
    associated boundaries.

    See Also:

    http://cfconventions.org/Data/cf-conventions/cf-conventions-1.7/
         cf-conventions.html#cell-boundaries

    https://github.com/pydata/xarray/issues/2565
    """


    # For all time variables with bounds
    for v in variables.values():
        attrs = v.attrs
        has_date_units = 'units' in attrs and 'since' in attrs['units']
        if has_date_units and 'bounds' in attrs:
            if attrs['bounds'] in variables:
                bounds_attrs = variables[attrs['bounds']].attrs
                bounds_attrs.setdefault('units', attrs['units'])
                if 'calendar' in attrs:
                    bounds_attrs.setdefault('calendar', attrs['calendar'])

I think we would just need to call this prior to calling encode_cf_variable()

I'm afraid it's not quite that simple :).

In cases where someone creates a datetime-like variable in memory (e.g. with pd.date_range in @mathause's example), unless they explicitly add encoding attributes, 'units' and 'calendar' will need to be computed on the fly. This happens inside of encode_cf_variable, currently on a per-variable basis, so trying to make sure the attributes are equal before that step will not help, because they might not exist.

Thanks @spencerkclark & @mathause. I now understand the issue better. #2921 was confusing in that @klindsay28 was trying to write an encoded dataset.

I've updated the tests to use @mathuse's example. The code now updates the time_bounds variable with the encoding 'units' and 'calendar' of the time variable. It also throws a warning when encoding.units is not specified for variables with a bounds attribute.

Do you think this is a good approach?